-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrated UserList Service #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just merged the latest dev code into this branch and ran the full test suite. I'm currently getting a few errors:
There were 8 failures:
1) VuFindTest\Mink\BulkTest::testBulkSave
Element not found: .modal-body .alert-success index 0
Failed asserting that null is of type "object".
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/BulkTest.php:206
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104
2) VuFindTest\Mink\CartTest::testCartSave
Element not found: .modal-body .alert-success index 0
Failed asserting that null is of type "object".
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/CartTest.php:619
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104
3) VuFindTest\Mink\FavoritesTest::testListTaggingToDisplayChannel with data set "case insensitive channel match" ('CHANNEL', array(array('channel'), false), false, true)
Element not found: .channel-title h2 index 0
Failed asserting that null is of type "object".
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FavoritesTest.php:764
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104
4) VuFindTest\Mink\FavoritesTest::testListTaggingToDisplayChannel with data set "case sensitive channel match" ('channel', array(array('channel'), false), true, true)
Element not found: .channel-title h2 index 0
Failed asserting that null is of type "object".
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FavoritesTest.php:764
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104
5) VuFindTest\Mink\FavoritesTest::testListTaggingToDisplayChannel with data set "case sensitive AND match" ('channel banana', array(array('channel', 'banana'), false), true, true)
Element not found: .channel-title h2 index 0
Failed asserting that null is of type "object".
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FavoritesTest.php:764
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104
6) VuFindTest\Mink\FavoritesTest::testListTaggingToDisplayChannel with data set "case sensitive OR match" ('channel', array(array('channel', 'banana'), false, 'OR'), true, true)
Element not found: .channel-title h2 index 0
Failed asserting that null is of type "object".
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FavoritesTest.php:764
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104
7) VuFindTest\Mink\FavoritesTest::testListTaggingToDisplayChannel with data set "case insensitive OR match" ('channel', array(array('chAnnEl', 'banana'), false, 'OR'), false, true)
Element not found: .channel-title h2 index 0
Failed asserting that null is of type "object".
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FavoritesTest.php:764
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104
8) VuFindTest\Mink\ListViewsTest::testFavoritesInTabMode
Element not found: #information_cd588d8723d65ca0ce9439e79755fa0a-content .savedLists ul index 0
Failed asserting that null is of type "object".
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/ListViewsTest.php:125
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104
FAILURES!
Tests: 2586, Assertions: 14549, Failures: 8, Skipped: 1.
Are you seeing the same thing on your end?
In any case, I will also begin reviewing the code shortly -- I just wanted to start by bringing the code up to date and checking on the status of the automated tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more small comments -- I'll keep doing incremental reviews as time permits.
module/VuFind/src/VuFind/Controller/Plugin/FavoritesFactory.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skellamp, as you reported in Teams, the bulk delete action on individual lists is currently failing. I've opened vufind-org#3202 to create an integration test to cover this behavior, since this was previously missing from the test suite.
I find that the easiest way to troubleshoot unexpected behavior in the lightbox is to use a browser plugin that disables Javascript -- then you can work through the interactions without the lightbox getting in the way, which makes it easier to see error messages, etc. In this instance, when I turned off Javascript in my browser and turned on development mode in VuFind, this is what I'm seeing when I try to delete favorites:
...so it looks like something is going wrong with the ownership check in \VuFind\Db\Service\UserListService::removeResourcesById().
Is that enough to help you make further progress? Please let me know if you need me to take a deeper look!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question about the latest refactoring...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks once again, @skellamp -- everything looks good now, and all tests are passing. I made a couple of very small simplifications today, and I will merge this now!
No description provided.